-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move worker's close() to dedicated worker and shared worker #1119
Conversation
As WorkerGlobalScope's close() method should not be exposed to one of its derived interfaces, ServiceWorkerGlobalScope, close() needs to be moved to the derived interfaces which explicitly requires it. This commit moves close() to DedicatedWorkerGlobalScope and SharedWorkerGlobalScope. Related issue: w3c/ServiceWorker#865
|
||
<li><p>Set the worker's <code>WorkerGlobalScope</code> object's <span | ||
data-x="dom-WorkerGlobalScope-closing">closing</span> flag to true. (This prevents any further | ||
tasks from being queued.)</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think instead this should become a generic algorithm that takes a global object that dedicated.close()
and shared.close()
can use. I don't like having two algorithms saying the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I added a commit that addressed this by extracting "close a worker". Please take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, much appreciated.
Extract close a worker algorithm that can be invoked by the close() method of a worker. At the time of this patch, a dedicated worker and a shared worker invoke it.
<p>When a script invokes the <dfn><code data-x="dom-DedicatedWorkerGlobalScope- | ||
close">close()</code></dfn> method on a <code>DedicatedWorkerGlobalScope</code> object, the user | ||
agent must <span>close a worker</span> with the <code>DedicatedWorkerGlobalScope</code> | ||
object.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roughly, this should read: "The close()
method, when invoked, must [close a worker] with this DedicatedWorkerGlobalScope
object." That matches the style we have been using. Analogous for the close()
method of SharedWorkerGlobalScope
objects.
Just wait.. didn't see the other comments than the first one. I'll ask a review after addressing them all. |
@annevk, I think it's ready for review. Please. |
<dd>Clones <var>message</var> and transmits it to the <code>Worker</code> object associated with | ||
the worker that is represented by this <var>dedicatedWorkerGlobal</var> object. | ||
<var>transfer</var> can be passed as a list of objects that are to be transfered rather than | ||
cloned.</dd> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh hey, thank you for adding this!
Instead of associated with the worker that is represented by this <var>dedicatedWorkerGlobal</var> object
just say associated with <var>dedicatedWorkerGlobal</var>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -96414,11 +96394,46 @@ interface <dfn>WorkerGlobalScope</dfn> : <span>EventTarget</span> { | |||
<p>All messages received by that port must immediately be retargeted at the | |||
<code>DedicatedWorkerGlobalScope</code> object.</p> | |||
|
|||
<dl class="domintro"> | |||
<dt><var>dedicatedWorkerGlobal</var> . <code subdfn data-x="dom-DedicatedWorkerGlobalScope- | |||
postMessage">postMessage</code>(<var>message</var> [, <var>transfer</var> ])</dt> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot have a newline here. You need to break before data-x.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Done.
Looks really great otherwise, it just failed on the build system. Hopefully at some point we have active integration with the build system so I don't have to tell you about it so late :-( |
\o/ |
1 similar comment
\o/ |
This tests fail on Chromium because blink already removed the API. This change is to follow the spec changes discussed here: whatwg/html#1119 ,and here is the blink work: http://crbug.com/611640
As WorkerGlobalScope's close() method should not be exposed to one of
its derived interfaces, ServiceWorkerGlobalScope, close() needs to be
moved to the derived interfaces which explicitly require it. This
commit moves close() to DedicatedWorkerGlobalScope and
SharedWorkerGlobalScope.
Related issue: w3c/ServiceWorker#865